-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PLUGIN-1849] Error Management for BigQuery Action plugin #1496
base: develop
Are you sure you want to change the base?
[PLUGIN-1849] Error Management for BigQuery Action plugin #1496
Conversation
throw new RuntimeException(queryJob.getStatus().getError().getMessage()); | ||
String error = queryJob.getStatus().getError().getMessage(); | ||
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
error, error, ErrorType.USER, false, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Job can fail on BigQuery due to many reasons like permission issues as well right? So, we need to decide based on error codes & error reason cannot directly say that it is a user error.
unknown is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
7c6135e
to
e8b7cd6
Compare
e8b7cd6
to
1b88f26
Compare
1b88f26
to
e0c7a9b
Compare
throw new RuntimeException(e); | ||
String error = String.format( | ||
"Failed to execute query with exponential backoff with message: %s", e.getMessage()); | ||
throw GCPErrorDetailsProviderUtil.getHttpResponseExceptionDetailsFromChain(new RuntimeException(e), error, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to re-wrap exception here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
protected void executeQueryWithExponentialBackoff(BigQuery bigQuery, | ||
QueryJobConfiguration queryConfig, ActionContext context) | ||
throws Throwable { | ||
void executeQueryWithExponentialBackoff(BigQuery bigQuery, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we changing the access modifier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's already a final class, so we cannot have any subclasses. So I think, there is no need to make it protected anymore. Please let me know in case I missed anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can make the access modifier private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is still not addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
c98fe24
to
5d4f7bc
Compare
} | ||
throw e; | ||
throw GCPErrorDetailsProviderUtil.getHttpResponseExceptionDetailsFromChain(e, e.getCause().getMessage(), | ||
ErrorType.SYSTEM, true, GCPUtils.BQ_SUPPORTED_DOC_URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String errorReason = String.format("Failed to execute query with message: %s", e.getMessage())
if (cause != null) {
errorReason = String.format("Failed to execute query with message: %s", e.getCause().getMessage())
}
throw GCPErrorDetailsProviderUtil.getHttpResponseExceptionDetailsFromChain(e == null ? e : e.getCause(), errorReason, ErrorType.UNKNOWN, true, GCPUtils.BQ_SUPPORTED_DOC_URL);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@@ -214,7 +239,9 @@ private void executeQuery(BigQuery bigQuery, QueryJobConfiguration queryConfig, | |||
if (RETRY_ON_REASON.contains(queryJob.getStatus().getError().getReason())) { | |||
throw new BigQueryJobExecutionException(queryJob.getStatus().getError().getMessage()); | |||
} | |||
throw new RuntimeException(queryJob.getStatus().getError().getMessage()); | |||
String error = queryJob.getStatus().getError().getMessage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String error = String.format("Failed to execute query with reason: %s, message: %s", queryJob.getStatus().getError().getReason(), queryJob.getStatus().getError().getMessage());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
throw new RuntimeException(queryJob.getStatus().getError().getMessage()); | ||
String error = queryJob.getStatus().getError().getMessage(); | ||
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
error, error, ErrorType.UNKNOWN, false, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dependency
should be true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
String error = String.format("The query result total rows should be \"1\" but is \"%d\"", | ||
queryResults.getTotalRows()); | ||
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
error, error, ErrorType.SYSTEM, true, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why errorType SYSTEM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
} catch (InterruptedException e) { | ||
String errorMessage = String.format("The query job was interrupted: %s", e.getMessage()); | ||
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
errorMessage, errorMessage, ErrorType.SYSTEM, true, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ErrorType should be UNKNOWN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
nonExistingColumnNames)); | ||
nonExistingColumnNames); | ||
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
error, error, ErrorType.USER, true, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dependency should be false
here.
Dependency is true
only when error is returned from external source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add screenshot evidences for BQExecute
plugin always.
String error = String.format("The query result total rows should be \"1\" but is \"%d\"", | ||
queryResults.getTotalRows()); | ||
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
error, error, ErrorType.USER, true, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment dependency
will be false
. dependency
is true
only for errors returned from external library clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
} | ||
throw e; | ||
throw GCPErrorDetailsProviderUtil.getHttpResponseExceptionDetailsFromChain( | ||
e == null ? e : (Exception) e.getCause(), errorReason, ErrorType.UNKNOWN, true, GCPUtils.BQ_SUPPORTED_DOC_URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to cast into Exception
here, instead accept Throwable
here instead of Exception
:
google-cloud/src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProviderUtil.java
Line 72 in 77e0ac8
public static ProgramFailureException getHttpResponseExceptionDetailsFromChain(Exception e, String errorReason, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
src/test/java/io/cdap/plugin/gcp/bigquery/action/BigQueryExecuteTest.java
Show resolved
Hide resolved
19f7e9a
to
1a7ee03
Compare
throw new RuntimeException(queryJob.getStatus().getExecutionErrors().toString()); | ||
String error = queryJob.getStatus().getExecutionErrors().toString(); | ||
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
error, error, ErrorType.UNKNOWN, true, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a table based on error type & error message here: https://cloud.google.com/bigquery/docs/error-messages#errortable
ErrorType
will not unknown always, this comment applies to whole PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added and updated
} catch (InterruptedException e) { | ||
String errorMessage = String.format("The query job was interrupted: %s", e.getMessage()); | ||
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
errorMessage, errorMessage, ErrorType.UNKNOWN, false, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dependency
should be true
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
src/main/java/io/cdap/plugin/gcp/bigquery/action/BigQueryArgumentSetter.java
Show resolved
Hide resolved
if (e instanceof Exception) { | ||
String error = | ||
String.format("Failed to execute query with exponential backoff with message: %s", e.getMessage()); | ||
throw GCPErrorDetailsProviderUtil.getHttpResponseExceptionDetailsFromChain(e, error, | ||
ErrorType.USER, true, GCPUtils.BQ_SUPPORTED_DOC_URL); | ||
} | ||
String errorMessage = String.format("Failed to execute query with exponential backoff with message: %s", | ||
e.getMessage()); | ||
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
errorMessage, errorMessage, ErrorType.UNKNOWN, true, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, we can re-wrap exception into RuntimeException
and add it as cause instead of sending cause
as null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
throw new RuntimeException(e); | ||
String error = String.format("Failed to execute query with message: %s", e.getMessage()); | ||
throw GCPErrorDetailsProviderUtil.getHttpResponseExceptionDetailsFromChain(e, error, ErrorType.UNKNOWN, | ||
true, GCPUtils.GCS_SUPPORTED_DOC_URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why GCS_SUPPORTED_DOC_URL
for BQ errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add evidences of this new class being used and error getting propagated in UI correctly.
c682a83
to
554f6f5
Compare
try { | ||
queryJob.waitFor(); | ||
} catch (BigQueryException | InterruptedException e) { | ||
String errorMessage = String.format("The query job was interrupted, %s: %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bigquery query job failed, %s: %s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
*/ | ||
public class BigQueryErrorUtil { | ||
|
||
//https://cloud.google.com/bigquery/docs/error-messages#errortable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a space after //
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
errorMessage = String.format( | ||
"Resource was not found. Please verify the resource name. If the resource will be " | ||
+ "created at runtime, then update to use a macro for the resource name. " | ||
+ "Error message received was %s, %s", e.getClass().getName(), e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message received was %s: %s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
String error = queryJob.getStatus().getExecutionErrors().toString(); | ||
ErrorType type = BigQueryErrorUtil.getErrorType(queryJob.getStatus().getError().getReason()); | ||
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
error, error, type, true, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errorReason = String.format("The bigquery job failed with reason: %s", queryJob.getStatus().getError.getReason())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
a1d1fdf
to
2aecb46
Compare
ErrorType type = BigQueryErrorUtil.getErrorType(queryJob.getStatus().getError().getReason()); | ||
throw ErrorUtils.getProgramFailureException( | ||
new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorReason, errorReason, type, | ||
true, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why no supportDocUrl
added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
queryJob.getStatus().getError().getReason()); | ||
ErrorType type = BigQueryErrorUtil.getErrorType(queryJob.getStatus().getError().getReason()); | ||
throw ErrorUtils.getProgramFailureException( | ||
new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorReason, errorReason, type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errorMessage = queryJob.getStatus().getExecutionErrors().toString()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
LOG.error("The query job {} failed. Error: {}", jobId.getJob(), queryJob.getStatus().getError()); | ||
LOG.error( | ||
String.format("The query job %s failed with error %s and reason %s.", jobId.getJob(), | ||
queryJob.getStatus().getError().getReason(), queryJob.getStatus().getError())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String.format("The query job %s failed with reason: %s and error: %s.", jobId.getJob(), queryJob.getStatus().getError().getReason(), queryJob.getStatus().getExecutionErrors().toString()))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
ErrorType type = BigQueryErrorUtil.getErrorType(queryJob.getStatus().getError().getReason()); | ||
throw ErrorUtils.getProgramFailureException( | ||
new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), error, error, type, true, | ||
null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why no supportDocUrl
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
* @param errorReason the error reason to classify | ||
* @return the corresponding ErrorType (USER, SYSTEM, UNKNOWN) | ||
*/ | ||
public static String getErrorCode(String errorReason) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this method called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is used in BiqqueryArgumentSetter at line 119 ,
` if (queryJob.getStatus().getError() != null) {
String errorReason = String.format(
"The bigquery job failed with reason: %s. For more details, see %s",
queryJob.getStatus().getError().getReason(), GCPUtils.BQ_SUPPORTED_DOC_URL);
ErrorType type = BigQueryErrorUtil.getErrorType(queryJob.getStatus().getError().getReason());
String errorCode = BigQueryErrorUtil.getErrorCode(
queryJob.getStatus().getError().getReason());
throw ErrorUtils.getProgramFailureException(
new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorReason,
queryJob.getStatus().getExecutionErrors().toString(), type, true, ErrorCodeType.HTTP,
errorCode, GCPUtils.BQ_SUPPORTED_DOC_URL, null);
}`
ERROR_REASON_TO_ERROR_CODE.put("invalid", 400); | ||
ERROR_REASON_TO_ERROR_CODE.put("invalidQuery", 400); | ||
ERROR_REASON_TO_ERROR_CODE.put("billingTierLimitExceeded", 400); | ||
ERROR_REASON_TO_ERROR_CODE.put("resourceInUse", 400); | ||
ERROR_REASON_TO_ERROR_CODE.put("resourcesExceeded", 400); | ||
ERROR_REASON_TO_ERROR_CODE.put("badRequest", 400); | ||
ERROR_REASON_TO_ERROR_CODE.put("invalidUser", 400); | ||
ERROR_REASON_TO_ERROR_CODE.put("notFound", 404); | ||
ERROR_REASON_TO_ERROR_CODE.put("duplicate", 409); | ||
ERROR_REASON_TO_ERROR_CODE.put("accessDenied", 403); | ||
ERROR_REASON_TO_ERROR_CODE.put("billingNotEnabled", 403); | ||
ERROR_REASON_TO_ERROR_CODE.put("quotaExceeded", 403); | ||
ERROR_REASON_TO_ERROR_CODE.put("rateLimitExceeded", 403); | ||
ERROR_REASON_TO_ERROR_CODE.put("responseTooLarge", 403); | ||
ERROR_REASON_TO_ERROR_CODE.put("blocked", 403); | ||
ERROR_REASON_TO_ERROR_CODE.put("proxyAuthenticationRequired", 407); | ||
|
||
// System Errors | ||
ERROR_REASON_TO_ERROR_CODE.put("tableUnavailable", 400); | ||
ERROR_REASON_TO_ERROR_CODE.put("backendError", 503); | ||
ERROR_REASON_TO_ERROR_CODE.put("internalError", 500); | ||
ERROR_REASON_TO_ERROR_CODE.put("notImplemented", 501); | ||
ERROR_REASON_TO_ERROR_CODE.put("jobBackendError", 400); | ||
ERROR_REASON_TO_ERROR_CODE.put("jobInternalError", 400); | ||
ERROR_REASON_TO_ERROR_CODE.put("jobRateLimitExceeded", 400); | ||
ERROR_REASON_TO_ERROR_CODE.put("timeout", 400); | ||
|
||
// Unknown Errors | ||
ERROR_REASON_TO_ERROR_CODE.put("stopped", 200); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so , should we pass null in error code, instead of checking here
` if (queryJob.getStatus().getError() != null) {
String errorReason = String.format(
"The bigquery job failed with reason: %s. For more details, see %s",
queryJob.getStatus().getError().getReason(), GCPUtils.BQ_SUPPORTED_DOC_URL);
ErrorType type = BigQueryErrorUtil.getErrorType(queryJob.getStatus().getError().getReason());
String errorCode = BigQueryErrorUtil.getErrorCode(
queryJob.getStatus().getError().getReason());
throw ErrorUtils.getProgramFailureException(
new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorReason,
queryJob.getStatus().getExecutionErrors().toString(), type, true, ErrorCodeType.HTTP,
errorCode, GCPUtils.BQ_SUPPORTED_DOC_URL, null);
}`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, you can pass both errorCode & errorCodeType as null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, updated. and removed the error codes form util class also
} | ||
throw e; | ||
throw GCPErrorDetailsProviderUtil.getHttpResponseExceptionDetailsFromChain( | ||
e == null ? e : e.getCause(), errorReason, ErrorType.UNKNOWN, true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.getCause() == null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
src/main/java/io/cdap/plugin/gcp/bigquery/action/BigQueryArgumentSetter.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/gcp/bigquery/action/BigQueryArgumentSetter.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/gcp/bigquery/action/BigQueryExecute.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/gcp/bigquery/common/BigQueryErrorUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/gcp/bigquery/common/BigQueryErrorUtil.java
Outdated
Show resolved
Hide resolved
4bc048c
to
5821daa
Compare
https://cdap.atlassian.net/browse/PLUGIN-1849
Big Query Execute